fix: accept comma-separated values on deployment target and scope flags - #692

Blank-drop can silently flip a tenanted deploy to untenanted. ExpandCommaSeparated drops blank entries and returns nil when everything was blank, and pkg/executor/release.go:178 routes on isTenanted := len(params.Tenants) > 0 || len(params.TenantTags) > 0.

Concrete scenario (CI is exactly where this happens): --tenant "$TENANT_A,$TENANT_B" with both variables unset/empty yields "," → expands to nil → the CLI silently submits an untenanted deployment to the environment. Before this PR the literal "," (or "") was sent as a tenant name and the server rejected it. Same class of change for --exclude-deployment-target "$X" with $X empty: the exclusion list silently becomes empty instead of erroring.

Suggest erroring (or at least warning) when a flag value expands to nothing but the flag was explicitly provided, e.g. check cmd.Flags().Changed(name) && len(expanded) == 0.

Actioned in c64ab93. The routing claim still holds on this branch — pkg/executor/release.go:178 is isTenanted := len(params.Tenants) > 0 || len(params.TenantTags) > 0 — so an all-blank --tenant would have submitted an untenanted deployment.

I went with an error out of the expansion rather than the cmd.Flags().Changed(name) && len(expanded) == 0 check. ExpandCommaSeparated now fails on any component that is blank after trimming, and ExpandCommaSeparatedFlags propagates it as the first statement of deployRun/runbookRun:

--tenant has a blank value; check for an empty variable or a stray comma in ","

Reasons for that shape over the Changed check: it also catches the partial case ("$A,$B" with only $B empty), which len(expanded) == 0 misses and which narrows the deployment scope in exactly the same silent way; and it doesn't need each command to know which flags were explicitly provided. Error rather than warning because a blank component always means a caller-side substitution produced nothing — there is no invocation where "ignore it" is the right answer.

Covered by TestExpandCommaSeparated_RejectsBlankValues (empty string, lone comma, whitespace-only, blank in the middle, trailing comma, blank alongside a good repeat) and end-to-end by "release deploy rejects a blank comma-separated value rather than silently dropping it" in deploy_test.go.

One thing worth recording: the legacy aliases are covered too, but by accident of layering rather than by design. I drove util.ApplyFlagAliases with a throwaway test — --deployTo "," arrives on the primary flag as []string{"", ""}, and the expansion then rejects it. That only works because the expansion runs after alias application, which is the main reason I did not move the split to parse time; see my reply on the altitude thread.

No escape hatch for names containing commas — and interactive mode now emits automation commands that re-split them. The split is unconditional, so a tenant/target/environment named e.g. Foo, Inc can no longer be passed through the primary flags at all (the PR description flags this; adding a data point on the second-order effect).

The sharper edge is the interactive echo: values chosen from a picker are backfilled into resolvedFlags and GenerateAutomationCmd emits --tenant 'Foo, Inc' (pkg/util/flag/flag.go []string case emits values verbatim, one flag per element). Pasting that "Automation Command" into CI now splits it into Foo and Inc — a hard error if those don't exist, or a deploy to the wrong tenants/targets if they do. So the interactive flow can now hand the user a re-runnable command that isn't re-runnable, with no workaround. Worth deciding on the escape-hatch question before shipping rather than after.

Actioned in b77c149, with one residual that I think you should look at before shipping.

\, now means a literal comma. splitOnUnescapedCommas walks the value and only treats a backslash specially when it immediately precedes a comma, so a backslash anywhere else is preserved verbatim and target names like DOMAIN\host are unaffected. On the interactive side, selections are escaped on the way into resolvedFlags with executionscommon.EscapeCommas — deploy.go:269-277, run.go:337-345 and 471-479, run_by_tag.go:324-326 — so the echoed Automation Command round-trips through the split instead of re-splitting a picked name. TestEscapeCommas_RoundTripsThroughExpand pins the round trip; "release deploy treats a backslash-escaped comma as part of the value" covers the flag path end to end (--deployment-target 'Web\, Prod,Other' → ["Web, Prod", "Other"]), plus unit cases in TestExpandCommaSeparated for the escaped/unescaped mix, DOMAIN\host, and a trailing backslash.

Residual: the escape hatch only works on the primary flags, not the legacy aliases. Those are registered with util.AddFlagAliasesStringSlice, so pflag CSV-parses them before we see them. I drove ApplyFlagAliases with a throwaway test: --deployTo 'Web\, Prod' lands on the primary flag as []string{"Web\\", "\" Prod\""} — mangled, with a stray backslash and csv quote characters, not even cleanly split. Same for --env, --tag, --tenantTag, --target, --specificMachines, --exclude-target, --excludeMachines. This is pre-existing — those aliases have always CSV-split and have never been able to carry a comma — so it is not a regression from this PR, but --target is a commonly used alias and we now document an escape that silently doesn't work there. Fixing it properly means registering those aliases as StringArray and routing them through the same expansion, which in turn needs ApplyFlagAliases's Value.String() round trip fixed (it reconstructs slice values by naively splitting the bracketed string, so any value containing a comma is lossy regardless). I left that out as bigger than #556.

On the design question you raised: I think \, is the right escape rather than a quoting form, because quoting can't survive the shell — by the time we see the value the user's quotes are gone, so an inner quote convention would need its own escape anyway. The flag help for all five flags now says "or as a comma-separated list; escape a comma inside a value as '\,'".

Cross-reference: the comment on #703 (pkg/executionscommon/executionscommon.go:309) is the same finding. b77c149 is already an ancestor of nj/tier1-integration-tests, so that branch has the escape hatch — same answer there, including the alias caveat above. I have not touched #703's branch.

Open question for you: do you want the alias flags brought onto the same escaping path in this PR, or is "escape hatch on the primary flags only, legacy aliases stay CSV" the shipping position?

Altitude: the expansion lives in two run functions rather than the flag layer. Two costs:

1. Same-named flags now behave differently across commands: tenant connect --environment/-e (pkg/cmd/tenant/connect/connect.go:108) still does not split commas, so -e "dev,test" works on release deploy but sends the literal string on tenant connect.
2. Mutating flags.X.Value at the top of the run function creates an ordering dependency — any future code reading these flags in PreRunE or before these lines sees unsplit values, and every new command must remember to add the block.

A parse-time mechanism (a small splitting pflag.Value wrapper, or a util.StringArrayCommaSeparated(...) registration helper next to AddFlagAliasesStringSlice in pkg/util/pflagaliases.go) would give every command the behavior consistently and remove the ordering hazard.

Both sub-points hold, but I've left the expansion where it is and want your call, because the obvious parse-time version silently undoes the blank-value fix from c64ab93.

Point 1 confirmed. tenant connect registers -e/--environment with StringArrayVarP (connect.go:108) and does no splitting, so -e "dev,test" is one environment name there and two on release deploy.

Point 2 is real but currently latent. The only PreRunE on either command is util.ApplyFlagAliases, and the expansion is the first statement of deployRun/runbookRun, so nothing reads unsplit values today. It is an invariant held by convention, which is your point.

Why I didn't move it. A splitting pflag.Value has to reject blanks in Set, and util.ApplyFlagAliases discards Set errors — pflagaliases.go:63 and :66 are both _ = primaryFlag.Value.Set(...). I checked both shapes with a throwaway test driving ApplyFlagAliases:

• with a Value that errors on a blank component, --deployTo "," leaves the primary flag nil and no error escapes;
• with the expansion where it is now, the same input arrives as []string{"", ""} and fails with --environment has a blank value; ....

So parse-time splitting reintroduces exactly the silent scope change from the blank-drop thread, on the legacy alias path (--deployTo, --env, --tag, --tenantTag, --target, --specificMachines, --exclude-target, --excludeMachines).

The version that doesn't regress is three parts:

1. util.ApplyFlagAliases returns error. 7 call sites (buildinformation upload, package upload, release create, release deploy, release progression allow, release progression prevent, runbook run) — each is already inside a PreRunE that returns error, so it is one line each. Checked the other alias Set paths: string/stringArray Set never fail and bool aliases are fed from a bool flag's own String(), so the new splitting Value would be the only thing that can error.
2. A registration helper next to AddFlagAliasesStringSlice wrapping a splitting/blank-rejecting pflag.Value (keeping Type() == "stringArray" so help output and completion don't change; nothing in the repo calls GetStringArray, so that's free).
3. The split/escape logic moves out of executionscommon into pkg/util or a leaf package, because pkg/util can't import executionscommon.

I think (1) is worth doing on its own merits — swallowing Set errors during alias application is a latent bug independent of this PR. What I don't want to do unilaterally is (2)+(3) plus opting the other commands into comma splitting, under a PR scoped to the deploy/runbook scope flags — there are 11 StringArray --environment flags in pkg/cmd (these two, tenant connect, and the eight account ... create commands), and changing the rest is an observable behaviour change to commands #556 doesn't mention.

Decision I need: land #556 with the expansion in the two run functions and take (1)+(2)+(3) as a follow-up that also decides which other commands opt in — or do you want all of it here, and if so should the splitting Value go on every StringArray --environment or only on the deploy/runbook ones?

Minor: this five-line block is duplicated verbatim from deployRun. If the flag set grows (the PR description already floats --runbook-tag), the two lists have to be kept in sync by hand. A tiny shared helper would collapse both call sites, e.g. executionscommon.ExpandCommaSeparatedAll(&flags.Environments.Value, &flags.Tenants.Value, ...) taking ...*[]string.

Actioned in 8c07b92. executionscommon.ExpandCommaSeparatedFlags(...*flag.Flag[[]string]) expands in place, so both call sites are now one call: deploy.go:202 and run.go:205.

I took *flag.Flag[[]string] rather than the ...*[]string you suggested because the flags carry their own Name, and that turned out to be load-bearing once blanks became an error (c64ab93, on the other thread) — the helper can say --tenant has a blank value instead of a message with no idea which flag it came from. A ...*[]string signature would have needed the name passed alongside each pointer, which is the two-lists-to-keep-in-sync problem again.

Covered by TestExpandCommaSeparatedFlags (mutation in place across several flags, and the error naming the offending flag).